feat(mcp): an agent can finally read back what it recorded to the SD card - #511

• Expose SD-card retrieval over MCP: list files, check storage, download logs, and delete files.
• Optionally parse downloaded logs into CSV and return row/sample counts plus parse warnings.
• Fix Core SD config snapshotting to avoid concurrent channel enumeration failures.

graph TD
  A["MCP Client"] --> B["MCP Tools"] --> C["DaqifiAgent"] --> D["Core SD APIs"] --> E{{"DAQiFi + SD"}}
  C --> F[("Temp files")]
  C --> G["Parse + CSV export"] --> F
  subgraph Legend
    direction LR
    _ext{{"Hardware"}} ~~~ _svc["Code module"] ~~~ _fs[("Local FS")]
  end

Loading

The following are alternative approaches to this PR:

1. Split download vs export into separate MCP tools

• ➕ Clearer semantics: download is always pure transfer; export is explicit post-processing
• ➕ Allows retries of CSV parsing without re-downloading large files
• ➖ More tool calls and more state to pass around (raw path + device-side filename + config hints)
• ➖ Agents may forget the second step; current default closes the loop in one call

2. Return file bytes via MCP resources/attachments instead of temp paths

• ➕ Avoids relying on local temp paths and file accessibility assumptions
• ➕ Potentially more portable across deployments/containers
• ➖ May be impractical for large logs (memory/transport overhead)
• ➖ Requires additional MCP protocol/resource implementation complexity; temp-path handoff is simplest today

3. Gate download under --read-only like other SD actions

• ➕ Strict interpretation of read-only mode
• ➕ Avoids local disk writes in read-only deployments
• ➖ Conflicts with the intent of read-only as “safe observation”; prevents retrieving data needed for analysis
• ➖ Still requires some persistence mechanism to get bytes to the agent; current behavior documents the temp write clearly

Recommendation: Keep the PR’s approach: a single download tool that can optionally export CSV is the right UX for agents and minimizes repeated long transfers. The temp-file side effect is acceptable given it’s documented and required for handoff, and delete remains correctly gated by --read-only. The only follow-up worth considering is whether a future MCP “resource download” path is needed for deployments where local temp paths aren’t accessible to the client.

Enhancement (4) +526 / -0

DaqifiAgent.csImplement SD retrieval operations and CSV export pipeline +326/-0

Implement SD retrieval operations and CSV export pipeline

• Adds ListSdFilesAsync, GetSdStorageAsync, DownloadSdFileAsync, and DeleteSdFileAsync, including MCP-friendly exception rewriting and read-only enforcement only for destructive delete. Implements filename preflight resolution, snapshots live device config for timestamp parsing, and adds an ExportCsvAsync helper that detects format from device-side names and writes CSV next to the raw download while reporting counts/warnings.

src/Daqifi.Mcp/DaqifiAgent.cs

Dtos.csAdd DTOs for SD listing, storage, download, and delete results +59/-0

Add DTOs for SD listing, storage, download, and delete results

• Introduces SdFileEntry, SdFileListing, SdStorageReport, SdDownloadReport, and SdDeleteResult to structure tool outputs and encode important semantics like unknown size vs empty file and CSV error reporting.

src/Daqifi.Mcp/Dtos.cs

SdCardSampleSource.csAdd ISampleSource adapter to export parsed SD logs as CSV +106/-0

Add ISampleSource adapter to export parsed SD logs as CSV

• Implements an ISampleSource over parsed SdCardLogEntry streams so Core’s CsvExporter can write CSVs. Tracks sample count vs row count (timestamp-based), always exports a digital column, and reports dropped analog columns when log width exceeds known channel count.

src/Daqifi.Mcp/SdCardSampleSource.cs

DaqifiTools.csExpose SD retrieval tools via MCP tool surface +35/-0

Expose SD retrieval tools via MCP tool surface

• Adds MCP tool definitions for list_sd_files, get_sd_storage, download_sd_file (with exportCsv default true), and delete_sd_file with clear descriptions and GuardAsync wrapping.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

Bug fix (1) +11 / -2

SdCardDeviceConfiguration.csFix FromDevice to snapshot channels to avoid concurrent modification +11/-2

Fix FromDevice to snapshot channels to avoid concurrent modification

• Adds null guarding and switches enumeration from the live Channels view to GetChannelsSnapshot(). Ensures analog and digital counts come from the same snapshot and avoids "Collection was modified" failures.

src/Daqifi.Core/Device/SdCard/SdCardDeviceConfiguration.cs

Tests (2) +579 / -0

SdCardDeviceConfigurationTests.csAdd regression tests for SdCardDeviceConfiguration.FromDevice snapshotting +84/-0

Add regression tests for SdCardDeviceConfiguration.FromDevice snapshotting

• Introduces unit and concurrency/regression tests ensuring FromDevice validates null input, reports analog/digital counts, and does not throw when channels are repopulated concurrently.

src/Daqifi.Core.Tests/Device/SdCard/SdCardDeviceConfigurationTests.cs

SdCardToolsTests.csAdd MCP contract + regression tests for SD retrieval + CSV export +495/-0

Add MCP contract + regression tests for SD retrieval + CSV export

• Adds extensive tests covering read-only gating, unknown device messaging, filename validation and resolution, DTO behavior, SdCardSampleSource semantics, and end-to-end CSV export against synthetic on-disk logs.

src/Daqifi.Mcp.Tests/SdCardToolsTests.cs

Documentation (2) +21 / -5

README.mdMention MCP SD data retrieval in top-level README +2/-2

Mention MCP SD data retrieval in top-level README

• Updates the project overview text to clarify that the MCP server can retrieve SD logs back (including CSV conversion), not only start/stop SD logging.

README.md

README.mdDocument new SD tools and read-only behavior +19/-3

Document new SD tools and read-only behavior

• Adds tool documentation for SD listing/storage/download/delete, clarifies that downloads write to local temp, and documents operational guidance (retrieve before streaming) plus which operations are blocked by --read-only.

src/Daqifi.Mcp/README.md

Description

ExportCsvAsync opens the CSV output FileStream with default (non-async) options while
CsvExporter.ExportAsync performs many WriteAsync calls; this can fall back to synchronous file
I/O / thread-pool-assisted writes and reduce scalability and cancellation responsiveness during
large exports.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R865-868]

+                var fileStream = new FileStream(csvPath, FileMode.Create, FileAccess.Write, FileShare.Read);
+                await using (fileStream.ConfigureAwait(false))
+                {
+                    var writer = new StreamWriter(fileStream);

Relevance

●●● Strong

They favor true-async patterns; output stream should match async exporter writes for
scalability/cancellation.

PR-#94
PR-#400

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code creates the CSV output stream using the non-async FileStream constructor, but the
exporter uses async write APIs throughout, so the export path is not truly async on the output side.

src/Daqifi.Mcp/DaqifiAgent.cs[865-873]
src/Daqifi.Core/Logging/Export/CsvExporter.cs[28-33]
src/Daqifi.Core/Logging/Export/CsvExporter.cs[90-112]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ExportCsvAsync` writes CSV using `CsvExporter.ExportAsync(...)` which uses async `TextWriter.WriteAsync` heavily, but the CSV output `FileStream` is created without `useAsync: true` (and without an explicit buffer size). For large SD logs, this can degrade throughput and responsiveness because the async export can end up performing synchronous file writes (often via thread-pool assistance), and cancellation may be less responsive.

### Issue Context
Input reading already uses `useAsync: true`; output should be consistent.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[865-874]

### Proposed fix
Create the CSV `FileStream` with `useAsync: true` and an explicit buffer size, e.g.:
```csharp
var fileStream = new FileStream(
   csvPath,
   FileMode.Create,
   FileAccess.Write,
   FileShare.Read,
   bufferSize: 64 * 1024, // or a shared constant
   useAsync: true);
```
Optionally also construct `StreamWriter` with an explicit encoding/buffer if desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

ExportCsvAsync creates localPath + ".csv" before exporting; if CSV export/writes throw, the
partially-written CSV is left on disk. DownloadSdFileAsync then swallows the exception and returns
CsvPath=null, so callers can’t discover or clean up the orphaned temp file.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R857-860]

+            var csvPath = localPath + ".csv";
+            var fileStream = new FileStream(csvPath, FileMode.Create, FileAccess.Write, FileShare.Read);
+            await using (fileStream.ConfigureAwait(false))
+            {

Relevance

●●● Strong

Temp-file cleanup on export failure is a straightforward reliability fix; consistent with existing
CSV deletion on zero-samples.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CSV is created with FileMode.Create and exported into, but only the zero-sample case deletes
it; other exceptions propagate. The caller-level tool catches export exceptions and returns without
a CSV path, which makes any partially-created CSV file impossible for the caller to clean up.

src/Daqifi.Mcp/DaqifiAgent.cs[857-878]
src/Daqifi.Mcp/DaqifiAgent.cs[695-713]
src/Daqifi.Mcp/Dtos.cs[214-233]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ExportCsvAsync` can leave a partially written `*.csv` file behind when `CsvExporter.ExportAsync(...)` (or stream writing) throws. Since `DownloadSdFileAsync` catches and converts those failures into a successful tool response with `CsvPath=null`, the orphaned file becomes undiscoverable and accumulates in temp storage.

### Issue Context
- The CSV file is created eagerly (`FileMode.Create`) and only deleted in the special `SampleCount == 0` case.
- Any other exception during the export path leaves the file on disk.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[857-889]
- src/Daqifi.Mcp/DaqifiAgent.cs[695-713]

### Implementation notes
- Wrap the CSV creation/export block in a `try/catch` that calls `TryDelete(csvPath)` on *any* exception (including IO exceptions).
- Prefer writing to a temp name (e.g., `localPath + ".csv.tmp"`) and `File.Move(..., overwrite:true)` only after a successful export; on failure, delete the temp.
- Keep `OperationCanceledException` behavior consistent (either delete the temp file before rethrow or ensure the temp-name approach prevents litter).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

CsvWriteBufferBytes duplicates the existing default parser buffer size
(SdCardParseOptions.BufferSize = 64 KB), creating a drift risk where future tuning changes one side
but not the other and leaves the "matching" comment inaccurate.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R82-85]

+    /// Write buffer for the CSV a download exports, matching the 64 KB default the SD parsers read
+    /// with so neither side of the export is the narrow one.
+    /// </summary>
+    private const int CsvWriteBufferBytes = 64 * 1024;

Relevance

●●● Strong

Team often accepts small constant hygiene/DRY fixes; avoids drift and keeps buffer-size intent
centralized.

PR-#420
PR-#422

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a new 64 KB constant in DaqifiAgent and uses it as the CSV output FileStream buffer,
while Core already defines the SD parser default buffer size as 64 KB via
SdCardParseOptions.BufferSize. These are separate definitions that can diverge over time.

src/Daqifi.Mcp/DaqifiAgent.cs[81-86]
src/Daqifi.Mcp/DaqifiAgent.cs[871-876]
src/Daqifi.Core/Device/SdCard/SdCardParseOptions.cs[16-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DaqifiAgent` introduces `CsvWriteBufferBytes = 64 * 1024` with a comment stating it matches the SD parser read buffer, but the parser buffer is already defined by `SdCardParseOptions.BufferSize` (default 64 KB). Keeping two separate defaults can drift and makes future performance tuning error-prone.

## Issue Context
- `ExportCsvAsync` already has access to `parseOptions.BufferSize` (used for the input stream). If the intent is to keep read/write buffers aligned, the write-side should be derived from the same source rather than a second constant.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[81-86]
- src/Daqifi.Mcp/DaqifiAgent.cs[871-876]

### Suggested change
- Remove `CsvWriteBufferBytes` and use `parseOptions.BufferSize` for the CSV output `FileStream` buffer size; OR
- If you want them independently tunable, rename/comment accordingly (remove the claim that it “matches” the parser), and consider centralizing defaults in a single shared constant.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

ResolveFileNameAsync interpolates the caller-provided fileName directly into an exception message,
but RequireFileName only trims/emptiness-checks. A filename containing control characters can
produce multiline/garbled MCP error output and confusing diagnostics.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R803-806]

+              + (files.Count > 20 ? $", and {files.Count - 20} more (call list_sd_files for all of them)." : ".");
+
+        throw new InvalidOperationException($"There is no file named '{fileName}' on the SD card. {available}");
+

Relevance

●●● Strong

Precedent: they accepted sanitizing user-supplied SD filenames to avoid unsafe output/injection;
same principle for exception text.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The thrown message includes the raw fileName parameter, and the only local validation is
trim/empty. Core’s SD filename validation rejects control characters for safety, highlighting that
the MCP-layer message can still echo such characters before Core is reached.

src/Daqifi.Mcp/DaqifiAgent.cs[800-806]
src/Daqifi.Mcp/DaqifiAgent.cs[918-927]
src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[1362-1374]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ResolveFileNameAsync` throws an `InvalidOperationException` that includes the raw `fileName` string. Since `RequireFileName` only trims and checks emptiness, a caller can supply control characters (e.g., `\n`, `\r`, tabs) that will be embedded into the exception message, potentially creating multiline/garbled output.

### Issue Context
Core already validates SD filenames for command-safety (`"`, `\n`, `\r`, `;`), but the "missing file" fast-fail occurs before Core validation and echoes the input into the error message.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[800-813]
- src/Daqifi.Mcp/DaqifiAgent.cs[918-927]

### Implementation notes
- Option A (preferred): strengthen `RequireFileName` to reject control chars and the same invalid characters Core rejects (at least `\r`, `\n`, `;`, `"`).
- Option B: escape/normalize the value used in the exception message (e.g., replace control chars with visible escape sequences) while still using the raw string for matching.
- Consider adding a small unit test that passes a filename with `\n` and asserts the thrown message does not contain a literal newline.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)